Skip to content

Document that GetUserProfileDirectory sets the size of the path written on success #1810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 22, 2025

Conversation

ChrisDenton
Copy link
Contributor

It was discovered that this undocumented behaviour was being relied on by a long deprecated Rust stdlib function.

In confirming this behaviour applied to both GetUserProfileDirectoryW and GetUserProfileDirectoryA, I found that the A function, unlike the W function, was not returning TRUE on success. Instead it also returns the size written to the buffer. I'm not sure if that part should be documented so I just changed the success case to nonzero and failure to zero.

Copy link

@ChrisDenton : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@workingjubilee
Copy link

Hello, @jwmsft seems to not be responding and this has lain fallow for a year. Can we get a review from someone else? @drewbatgit or @stevewhims perhaps?

@RalfJung
Copy link

Cc @cgettys-microsoft @sivadeilra -- this came up when discussing the semantics of a Miri shim.

It was discovered that this undocumented behaviour was being relied on by a long deprecated Rust stdlib function.

FWIW this function has been un-deprecated in the mean time.

@cgettys-microsoft
Copy link

cgettys-microsoft commented May 21, 2025

FWIW - I believe that the SAL annotations actually do match the semantics that the proposed documentation update suggests (edit: corrected misreading of the annotations) (from a recent userenv.h on my local dev machine - e.g. "C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um\UserEnv.h").
I.e. lpcchSize is always written with some value, and that lpProfileDir is annotated saying that how many characters it writes are stored in that field. (the opt just means you can pass in nullptr)

SAL reference: https://learn.microsoft.com/en-us/cpp/code-quality/annotating-function-parameters-and-return-values?view=msvc-170

//=============================================================================
//
// GetUserProfileDirectory
//
// Returns the path to the root of the requested user's profile
//
// hToken         -  User's token returned from LogonUser()
// lpProfileDir   -  Receives the path
// lpcchSize      -  Size of lpProfileDir
//
// Returns:  TRUE if successful
//           FALSE if not.  Call GetLastError() for more details
//
// Note:     If lpProfileDir is not large enough or NULL, the function will fail,
//           and lpcchSize will contain the necessary buffer size.
//
// Example return value: C:\Users\Joe
//
//=============================================================================

USERENVAPI
BOOL
WINAPI
GetUserProfileDirectoryA(
    _In_                            HANDLE  hToken,
    _Out_writes_opt_(*lpcchSize)    LPSTR lpProfileDir,
    _Inout_                         LPDWORD lpcchSize);
USERENVAPI
BOOL
WINAPI
GetUserProfileDirectoryW(
    _In_                            HANDLE  hToken,
    _Out_writes_opt_(*lpcchSize)    LPWSTR lpProfileDir,
    _Inout_                         LPDWORD lpcchSize);
#ifdef UNICODE
#define GetUserProfileDirectory  GetUserProfileDirectoryW
#else
#define GetUserProfileDirectory  GetUserProfileDirectoryA
#endif // !UNICODE

@cgettys-microsoft
Copy link

cgettys-microsoft commented May 21, 2025

Actually, I might be slightly misreading this - it may be that technically the SAL annotations don't go quite far enough - but that may just be because the annotations are pretty verbose to be that precise with:
E.g. _Out_writes_to_opt_(_Old_(*lpcchSize), *lpcchSize) might be the contract you want to have documented
Such is the challenge of writing in static analysis annotations rather than having language support (and maybe I'm missing a cleaner set of annotations for this...)

@sivadeilra
Copy link

The implementation meets the requirement that you want -- on successful return, *lpcchSize is the number of WCHAR elements that have been written, including the NUL at the end.

This is strongly implied by the SAL annotations. LPWSTR itself has a SAL annotation that specifies that the pointed-to string is NUL terminated and is well-formed after function return. The _Out_writes_opt_ indicates (on the successful return) that *lpcchSize is the number of elements written to lpProfileDir. Taken together, these two invariants correctly imply that the returned string is both NUL terminated and that *lpcchSize contains its length (or is greater, which is fine).

I've checked the implementation and it guarantees this contract.

@drewbatgit drewbatgit merged commit 91aec65 into MicrosoftDocs:docs May 22, 2025
1 check passed
@ChrisDenton ChrisDenton deleted the getuserprofiledirectoryw branch May 22, 2025 18:45
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 23, 2025
… r=ChrisDenton

GetUserProfileDirectoryW is now documented to always store the size

Update to match MicrosoftDocs/sdk-api#1810

Also fix a bug in the Miri implementation while I am starting at that code...

r? `@ChrisDenton`
Fixes rust-lang#141254
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 24, 2025
… r=ChrisDenton

GetUserProfileDirectoryW is now documented to always store the size

Update to match MicrosoftDocs/sdk-api#1810

Also fix a bug in the Miri implementation while I am starting at that code...

r? ``@ChrisDenton``
Fixes rust-lang#141254
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 24, 2025
… r=ChrisDenton

GetUserProfileDirectoryW is now documented to always store the size

Update to match MicrosoftDocs/sdk-api#1810

Also fix a bug in the Miri implementation while I am starting at that code...

r? ```@ChrisDenton```
Fixes rust-lang#141254
rust-timer added a commit to rust-lang/rust that referenced this pull request May 24, 2025
Rollup merge of #141405 - RalfJung:GetUserProfileDirectoryW, r=ChrisDenton

GetUserProfileDirectoryW is now documented to always store the size

Update to match MicrosoftDocs/sdk-api#1810

Also fix a bug in the Miri implementation while I am starting at that code...

r? ```@ChrisDenton```
Fixes #141254
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request May 25, 2025
…enton

GetUserProfileDirectoryW is now documented to always store the size

Update to match MicrosoftDocs/sdk-api#1810

Also fix a bug in the Miri implementation while I am starting at that code...

r? ```@ChrisDenton```
Fixes #141254
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request May 26, 2025
… r=ChrisDenton

GetUserProfileDirectoryW is now documented to always store the size

Update to match MicrosoftDocs/sdk-api#1810

Also fix a bug in the Miri implementation while I am starting at that code...

r? ```@ChrisDenton```
Fixes rust-lang#141254
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants